feature/basis for filtering cards by distance without external maps#8
feature/basis for filtering cards by distance without external maps#8akshaygajjala1 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial end-to-end path for distance-based study-spot ordering by introducing user location capture in the Expo app and a backend API endpoint that sorts spots by haversine distance.
Changes:
- Add
latitude/longitudefields to theStudySpotmodel and expose them viato_dict(). - Add a
/api/study_spots/sort_by_distanceendpoint that returns spots sorted by computed distance. - Update the frontend to request location permissions and call the new endpoint; add
expo-locationand adjust TS JSX settings.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| instance/longhorn_studies.db | Adds a SQLite DB artifact containing a study_spots schema snapshot. |
| frontend/tsconfig.json | Sets jsx to react-jsx. |
| frontend/package.json | Adds expo-location dependency. |
| frontend/app/(tabs)/index.tsx | Requests user location and posts coordinates to the backend to fetch distance-sorted spots. |
| backend/routes.py | Introduces /study_spots/sort_by_distance and haversine distance helper. |
| backend/models.py | Adds latitude/longitude columns and includes them in API serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Get all study spots | ||
| spots = StudySpot.query.all() | ||
|
|
||
| # Calculate distances and sort | ||
| spots_with_distance = [] | ||
| for spot in spots: | ||
| # Skip spots without coordinates | ||
| if not hasattr(spot, 'latitude') or not hasattr(spot, 'longitude'): | ||
| continue | ||
| if spot.latitude is None or spot.longitude is None: | ||
| continue | ||
|
|
||
| distance = _haversine_distance(user_lat, user_lng, spot.latitude, spot.longitude) | ||
| spot_dict = spot.to_dict() | ||
| spot_dict['distance_from_user'] = distance | ||
| spots_with_distance.append(spot_dict) | ||
|
|
||
| # Sort by distance (closest first) | ||
| spots_with_distance.sort(key=lambda x: x['distance_from_user']) |
There was a problem hiding this comment.
StudySpot.query.all() loads every row into memory and then sorts in Python. This won’t scale and can become a latency/memory issue as the table grows. Consider filtering out null coordinates at the DB level, selecting only needed columns, and optionally supporting paging/limits.
| useEffect(() => { | ||
| if (userLat && userLng) { | ||
| fetchSortedByDistance(); | ||
| } |
There was a problem hiding this comment.
Same falsy-check issue here: if (userLat && userLng) will skip when either coordinate is 0. Prefer explicit null/undefined checks before calling fetchSortedByDistance().
|
|
||
| const [userLat, setUserLat] = useState<number | null>(null); | ||
| const [userLng, setUserLng] = useState<number | null>(null); | ||
| const [spots, setSpots] = useState<any[]>([]); |
There was a problem hiding this comment.
spots state is set but never read/rendered in this component, which is likely to trigger no-unused-vars lint noise and makes it harder to see intended behavior. Either render/use spots (even temporarily) or remove the state until it’s needed.
| # Skip spots without coordinates | ||
| if not hasattr(spot, 'latitude') or not hasattr(spot, 'longitude'): | ||
| continue | ||
| if spot.latitude is None or spot.longitude is None: | ||
| continue | ||
|
|
||
| distance = _haversine_distance(user_lat, user_lng, spot.latitude, spot.longitude) | ||
| spot_dict = spot.to_dict() | ||
| spot_dict['distance_from_user'] = distance | ||
| spots_with_distance.append(spot_dict) | ||
|
|
||
| # Sort by distance (closest first) | ||
| spots_with_distance.sort(key=lambda x: x['distance_from_user']) |
There was a problem hiding this comment.
This endpoint relies on StudySpot.latitude/longitude, but the create/update JSON handler (_study_spot_from_json) doesn’t accept/populate those fields. As a result, newly created/updated spots will have null coordinates and will be filtered out here, often returning an empty list. Add latitude/longitude handling (with validation) to the create/update flow, or adjust the endpoint’s behavior.
| # Skip spots without coordinates | |
| if not hasattr(spot, 'latitude') or not hasattr(spot, 'longitude'): | |
| continue | |
| if spot.latitude is None or spot.longitude is None: | |
| continue | |
| distance = _haversine_distance(user_lat, user_lng, spot.latitude, spot.longitude) | |
| spot_dict = spot.to_dict() | |
| spot_dict['distance_from_user'] = distance | |
| spots_with_distance.append(spot_dict) | |
| # Sort by distance (closest first) | |
| spots_with_distance.sort(key=lambda x: x['distance_from_user']) | |
| spot_dict = spot.to_dict() | |
| # If the spot doesn't have usable coordinates, include it with no distance | |
| if (not hasattr(spot, 'latitude') or not hasattr(spot, 'longitude') or | |
| spot.latitude is None or spot.longitude is None): | |
| spot_dict['distance_from_user'] = None | |
| spots_with_distance.append(spot_dict) | |
| continue | |
| # Compute distance for spots with valid coordinates | |
| distance = _haversine_distance(user_lat, user_lng, spot.latitude, spot.longitude) | |
| spot_dict['distance_from_user'] = distance | |
| spots_with_distance.append(spot_dict) | |
| # Sort by distance (closest first); spots without distance go last | |
| spots_with_distance.sort( | |
| key=lambda x: ( | |
| x['distance_from_user'] is None, | |
| x['distance_from_user'] if x['distance_from_user'] is not None else float('inf'), | |
| ) | |
| ) |
| user_lat = float(user_lat) | ||
| user_lng = float(user_lng) | ||
| except (ValueError, TypeError): | ||
| return jsonify({'error': 'Invalid coordinates'}), 400 |
There was a problem hiding this comment.
Coordinate validation here only checks “is present” and “can be cast to float”. It should also validate bounds (lat ∈ [-90, 90], lng ∈ [-180, 180]) to avoid nonsensical values and surprising distance calculations.
| return jsonify({'error': 'Invalid coordinates'}), 400 | |
| return jsonify({'error': 'Invalid coordinates'}), 400 | |
| # Validate coordinate bounds | |
| if not (-90 <= user_lat <= 90) or not (-180 <= user_lng <= 180): | |
| return jsonify({'error': 'Coordinates out of range'}), 400 |
| } | ||
|
|
||
| async function fetchSortedByDistance() { | ||
| if (!userLat || !userLng) return; |
There was a problem hiding this comment.
if (!userLat || !userLng) return; treats 0 as “missing”, which is incorrect for valid coordinates. Use explicit null/undefined checks (e.g., userLat == null || userLng == null) to decide whether coordinates are available.
| if (!userLat || !userLng) return; | |
| if (userLat == null || userLng == null) return; |
| const response = await fetch('http://localhost:8000/api/study_spots/sort_by_distance', { | ||
| method: 'POST', |
There was a problem hiding this comment.
Hard-coding http://localhost:8000 will fail on physical devices and many emulator/simulator setups because localhost resolves to the device itself. Consider centralizing the API base URL (env/config) and using platform-appropriate host resolution (e.g., dev machine LAN IP / Android emulator 10.0.2.2).
| user_lat: userLat, | ||
| user_lng: userLng | ||
| }) | ||
| }); |
There was a problem hiding this comment.
fetchSortedByDistance doesn’t check response.ok before calling response.json(), so non-2xx responses (or non-JSON error bodies) can throw or silently produce confusing UI state. Handle non-OK responses explicitly (and consider surfacing the API error message).
| }); | |
| }); | |
| if (!response.ok) { | |
| const errorText = await response.text().catch(() => ''); | |
| throw new Error( | |
| `Request failed with status ${response.status} ${response.statusText}${ | |
| errorText ? `: ${errorText}` : '' | |
| }`, | |
| ); | |
| } |
| const { status } = await Location.requestForegroundPermissionsAsync(); | ||
| if (status !== 'granted') { | ||
| return; | ||
| } | ||
|
|
||
| const location = await Location.getCurrentPositionAsync({}); | ||
| setUserLat(location.coords.latitude); | ||
| setUserLng(location.coords.longitude); |
There was a problem hiding this comment.
Location.getCurrentPositionAsync can throw (e.g., location services disabled, timeout). Wrap this call in try/catch and handle failures (at least log and avoid leaving state in an indeterminate null/partial state).
| const { status } = await Location.requestForegroundPermissionsAsync(); | |
| if (status !== 'granted') { | |
| return; | |
| } | |
| const location = await Location.getCurrentPositionAsync({}); | |
| setUserLat(location.coords.latitude); | |
| setUserLng(location.coords.longitude); | |
| try { | |
| const { status } = await Location.requestForegroundPermissionsAsync(); | |
| if (status !== 'granted') { | |
| console.warn('Location permission not granted'); | |
| setUserLat(null); | |
| setUserLng(null); | |
| return; | |
| } | |
| const location = await Location.getCurrentPositionAsync({}); | |
| if (!location || !location.coords) { | |
| console.error('Failed to obtain location coordinates'); | |
| setUserLat(null); | |
| setUserLng(null); | |
| return; | |
| } | |
| setUserLat(location.coords.latitude); | |
| setUserLng(location.coords.longitude); | |
| } catch (error) { | |
| console.error('Error getting user location:', error); | |
| setUserLat(null); | |
| setUserLng(null); | |
| } |
| latitude = db.Column(db.Float, nullable=True) | ||
| longitude = db.Column(db.Float, nullable=True) |
There was a problem hiding this comment.
Adding latitude/longitude columns requires a DB schema change. Since the app currently uses db.create_all() (which won’t alter existing tables), any existing SQLite DB will be incompatible and queries can fail. Consider adding a migration step / schema update script (or ensure the dev DB is recreated) as part of this change.
No description provided.